Feat/dataverse instantiate #422
Conversation
|
Warning Rate Limit Exceeded@amimart has exceeded the limit for the number of files or commits that can be reviewed per hour. Please wait 13 minutes and 10 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the @coderabbitai review command. Alternatively, push new commits to this PR. We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per repository. The provided content is comprehensive and well-structured. I will now generate the requested sections based on the provided instructions. WalkthroughThe updates involve enhancing a smart contract by adding a new Changes
TipsChat with CodeRabbit Bot (
|
size-limit report 📦
|
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #422 +/- ##
==========================================
- Coverage 96.94% 96.84% -0.11%
==========================================
Files 48 49 +1
Lines 9882 10003 +121
==========================================
+ Hits 9580 9687 +107
- Misses 302 316 +14 ☔ View full report in Codecov by Sentry. |
amimart
force-pushed
the
feat/dataverse-instantiate
branch
from
df655e8 to
87fd9a7
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files ignored due to filter (4)
- Cargo.lock
- Cargo.toml
- Makefile.toml
- contracts/okp4-dataverse/Cargo.toml
Files selected for processing (5)
- contracts/okp4-dataverse/src/contract.rs (3 hunks)
- contracts/okp4-dataverse/src/error.rs (1 hunks)
- contracts/okp4-dataverse/src/msg.rs (3 hunks)
- contracts/okp4-dataverse/src/state.rs (1 hunks)
- docs/okp4-dataverse.md (4 hunks)
Additional comments: 8
contracts/okp4-dataverse/src/error.rs (1)
- 4-11: The addition of the
Instantiate2Addresserror variant is a good practice for handling specific instantiation errors related to addresses. This aligns with the Rust convention of using enums for error handling and the use ofthiserrorfor deriving theErrortrait, which simplifies the implementation of error types.Ensure that the rest of the codebase is updated to handle this new error variant where relevant, especially in the instantiation logic of the smart contract.
contracts/okp4-dataverse/src/msg.rs (2)
4-12: The addition of
triplestore_configtoInstantiateMsgis a significant change. Ensure that all instantiations of the dataverse contract are updated to include this new configuration parameter. The documentation should also be updated to reflect this change.126-137: The
TripleStoreConfigstruct is well-defined and includes a default forlimitsusingokp4_cognitarium::msg::StoreLimitsInput::default. This is a good use of defaults to simplify configuration. However, ensure that theokp4_cognitariumdependency is correctly referenced and accessible in the project.docs/okp4-dataverse.md (5)
50-55: The documentation for
InstantiateMsghas been updated to include the newtriplestore_configparameter. This is a required parameter that specifies the configuration for the triple store associated with the dataverse instance. Thecode_idandlimitsfields withintriplestore_configare also documented, providing details on their purpose and default values. The default values forlimitsare quite large, which may be intended to indicate no practical limit, but it's important to ensure that these defaults are appropriate for the system's expected usage patterns and resource constraints.86-92: The documentation for
ExecuteMsg::RegisterDigitalResourcehas been updated to reflect the new parameters for registering a digital resource. It includes detailed descriptions of each parameter and their preconditions. The documentation is clear and provides the necessary information for understanding how to use the function.205-220: The
StoreLimitsInputsection has been updated to include descriptions of various limitations that can be applied to the triple store. The use of[Uint128](#uint128)for byte size and triple count fields suggests that the system is designed to handle very large numbers. It's important to ensure that the system has been tested to handle these limits and that the underlying infrastructure can support such configurations.226-236: The
TripleStoreConfigsection provides a detailed explanation of the configuration options available for the triple store. The documentation includes default values for each limit, which are the maximum values representable byUint128. This implies that, by default, there are no limits unless explicitly set. It's crucial to validate that these defaults align with the intended use cases and that they won't lead to potential resource exhaustion.245-270: The documentation for the
Uint64andUint128types has been added, including examples of how to use these types in code. This is useful for developers who need to work with large integers that exceed the safe range for JavaScript numbers. The examples provided are clear and demonstrate the usage effectively.
ccamel
left a comment
ccamel
left a comment
There was a problem hiding this comment.
Great job! 💪 There's just a minor comment I have, which I'll leave for you to review.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (3)
- contracts/okp4-dataverse/src/contract.rs (3 hunks)
- contracts/okp4-dataverse/src/msg.rs (3 hunks)
- docs/okp4-dataverse.md (4 hunks)
Additional comments: 10
docs/okp4-dataverse.md (5)
50-55: The documentation clearly explains the new parameters added to the
InstantiateMsg. It is important to ensure that thecode_idandlimitsare properly validated in the smart contract code to prevent any issues related to incorrect configuration or potential security vulnerabilities.86-92: The documentation for
ExecuteMsg::RegisterDigitalResourceis well-detailed, providing clear descriptions and preconditions for each parameter. It's crucial that the smart contract enforces these preconditions to maintain the integrity of the dataverse.210-222: The
TripleStoreConfigand its limits are well-documented. However, it's essential to ensure that the smart contract code properly enforces these limits to prevent abuse or denial of service attacks due to resource exhaustion. For instance, themax_triple_byte_sizeshould be checked to prevent the storage of excessively large triples.226-236: The
TripleStoreLimitsInputmirrors the structure ofTripleStoreConfiglimits. It's important to ensure that the input provided by users is validated against the expected ranges and types to prevent any runtime errors or misconfigurations.250-270: The documentation for
Uint128andUint64includes examples of how to use these types within the smart contract. It's important to ensure that the smart contract code handles these types correctly, especially considering JavaScript's limitations with large integers.contracts/okp4-dataverse/src/msg.rs (4)
4-12: The
InstantiateMsgstruct has been updated to include atriplestore_configfield. Ensure that all instantiations of the dataverse contract are updated to provide this configuration.58-76: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [14-75]
The
ExecuteMsgenum has been updated to reflect the change from registering datasets to registering digital resources. This is a significant change in functionality and should be clearly communicated to users of the contract to avoid confusion.
126-169: The
TripleStoreConfigandTripleStoreLimitsInputstructs have been added to configure the triple store. It's important to ensure that the limits set here are reasonable and that they are properly documented so that users understand the constraints of the triple store.171-198: The
Fromtrait implementation for convertingTripleStoreLimitsInputtookp4_cognitarium::msg::StoreLimitsInputis straightforward. However, it's important to ensure that theokp4_cognitariumcontract expects the same default values as set in theTripleStoreLimitsInputstruct whenNoneis provided.contracts/okp4-dataverse/src/contract.rs (1)
- 1-14: The import statements and contract version setting are standard and appear correct. The use of
concat!macro forCONTRACT_NAMEensures that the contract name is correctly set at compile time.
when linking a contract to another we may encounter C symbol duplication caused by the export of contract's entrypoints, we avoid that this way.
amimart
force-pushed
the
feat/dataverse-instantiate
branch
from
2211293 to
c142976
Compare
amimart
force-pushed
the
feat/dataverse-instantiate
branch
from
78baf5a to
31340f2
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files ignored due to filter (4)
- Cargo.lock
- Cargo.toml
- Makefile.toml
- contracts/okp4-dataverse/Cargo.toml
Files selected for processing (6)
- .size-limit.json (1 hunks)
- contracts/okp4-dataverse/src/contract.rs (3 hunks)
- contracts/okp4-dataverse/src/error.rs (1 hunks)
- contracts/okp4-dataverse/src/msg.rs (3 hunks)
- contracts/okp4-dataverse/src/state.rs (1 hunks)
- docs/okp4-dataverse.md (4 hunks)
Files skipped from review due to trivial changes (1)
- .size-limit.json
Additional comments: 13
contracts/okp4-dataverse/src/error.rs (1)
- 4-11: The addition of the
Instantiate2Addresserror variant is appropriate for handling instantiation errors related to a specific address. This follows the standard pattern of error handling in Rust, using thethiserrorcrate for deriving theErrortrait, which is a common practice in Rust for error management.Ensure that the rest of the codebase properly uses this new error variant where relevant, especially in the instantiation logic of the smart contract.
contracts/okp4-dataverse/src/msg.rs (3)
1-12: The
InstantiateMsgstruct has been updated to include atriplestore_configfield of typeTripleStoreConfig. This change allows the dataverse to be initialized with specific configuration settings for the triple store. Ensure that all parts of the code that create anInstantiateMsgare updated to include this new field.126-168: The
TripleStoreConfigandTripleStoreLimitsInputstructs have been introduced to specify the configuration and limitations of the triple store. It's important to ensure that the default values mentioned in the comments (e.g.,Uint128::MAXformax_triple_count) are actually set as defaults in the implementation if the fields areNone. If not, the code should be updated to reflect the correct defaults.171-198: The
Fromtrait implementation for convertingTripleStoreLimitsInputtookp4_cognitarium::msg::StoreLimitsInputis correctly checking forNonebefore setting the values. However, it's important to ensure that theokp4_cognitarium::msg::StoreLimitsInputstruct has the same fields and expects the same types. If there are any discrepancies, they should be addressed.contracts/okp4-dataverse/src/contract.rs (3)
- 14-14: The
CONTRACT_NAMEconstant is correctly using theconcat!macro to include the package name fromCARGO_PKG_NAME. This is a good practice for dynamically setting the contract name based on the package name.---end hunk 0---
---start hunk 1---
36-42: The conditional compilation for testing purposes is a good practice to mock behavior that depends on external factors not present in the test environment. However, ensure that the "predicted address" used in tests aligns with the actual behavior in production to avoid discrepancies.
52-63: The response construction is correct and follows best practices by adding attributes and messages that will be useful for debugging and interaction with the contract.
---end hunk 1---
---start hunk 2---
docs/okp4-dataverse.md (6)
50-55: The documentation clearly explains the new parameters added to the
InstantiateMsg. It is important to ensure that the descriptions are accurate and that theTripleStoreConfigand its nested parameters are implemented as described. The use of Markdown formatting for required fields is consistent and enhances readability.86-92: The documentation for
ExecuteMsg::RegisterDigitalResourceis well-structured and provides a clear explanation of the parameters and their requirements. The preconditions for theregister_digital_resource.identifierandregister_digital_resource.provided_byare particularly important for ensuring data integrity within the dataverse.205-240: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [205-246]
The
RdfFormatsection is informative and provides external references to the specifications for each format. This is useful for developers who may need to implement or interact with these formats. The inclusion ofNQuadsandNTriplesindicates support for standard RDF serialization formats, which is beneficial for interoperability.
210-222: The
TripleStoreConfigsection is crucial as it defines the configuration for the triple store. It is important to ensure that the default values mentioned (e.g.,[Uint128::MAX]) are correctly implemented in the code to prevent any unexpected behavior. The documentation should reflect the actual behavior of the system.226-236: The
TripleStoreLimitsInputsection is well-documented, providing clear descriptions and default values for each property. It is important to verify that the system correctly handles null values and defaults as described here.248-270: The
Uint128andUint64sections provide a brief explanation of these types. The example provided forUint64is helpful for understanding its usage. Ensure that the example code is tested and works as expected, as it is a part of the official documentation.
| pub const DATAVERSE: Item<'_, Dataverse> = Item::new("dataverse"); | ||
|
|
||
| #[derive(Serialize, Deserialize, Clone, Debug, PartialEq, Eq)] | ||
| pub struct Dataverse { | ||
| pub name: String, | ||
| pub triplestore_address: String, | ||
| } |
There was a problem hiding this comment.
The implementation of the Dataverse struct and the DATAVERSE constant is consistent with the summary provided. However, as previously mentioned, it is recommended to use the Addr type for the triplestore_address field to ensure that the address is validated and to avoid potential errors from handling it as a String.
#[derive(Serialize, Deserialize, Clone, Debug, PartialEq, Eq)]
pub struct Dataverse {
pub name: String,
pub triplestore_address: Addr, // Use Addr to ensure the address is valid
}| let salt = Binary::from(msg.name.as_bytes()); | ||
|
|
||
| /// Necessary stuff for testing purposes, see: https://github.com/CosmWasm/cosmwasm/issues/1648 | ||
| #[allow(unused)] |
There was a problem hiding this comment.
The #[allow(unused)] attribute should be removed if triplestore_address is being used, as it seems to be in this case.
- #[allow(unused)]| assert_eq!( | ||
| res.messages, | ||
| vec![SubMsg::new(WasmMsg::Instantiate2 { | ||
| admin: Some("cosmos2contract".to_string()), |
There was a problem hiding this comment.
The admin field in the Instantiate2 message should match the env.contract.address used in the actual instantiate function. If "cosmos2contract" is not the intended admin, this should be corrected to maintain consistency with the contract's logic.
- admin: Some("cosmos2contract".to_string()),
+ admin: Some(env.contract.address.to_string()),---end hunk 2---
amimart
force-pushed
the
feat/dataverse-instantiate
branch
2 times, most recently
from
54ef46a to
e2cf607
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- contracts/okp4-dataverse/src/contract.rs (3 hunks)
- contracts/okp4-dataverse/src/state.rs (1 hunks)
Additional comments: 5
contracts/okp4-dataverse/src/state.rs (2)
5-5: The constant
DATAVERSEis correctly defined usingItem::newwith the key "dataverse". This will create a storage item for theDataversestruct.7-11: The
Dataversestruct is well-defined with appropriate attributes and derives common traits such asSerialize,Deserialize,Clone,Debug,PartialEq, andEq. These traits are necessary for serialization/deserialization, cloning instances, debugging, and comparing instances respectively.contracts/okp4-dataverse/src/contract.rs (3)
1-14: The use of
concat!macro forCONTRACT_NAMEis a good practice as it ensures the name is determined at compile time, which is beneficial for performance and error prevention. However, ensure that theCONTRACT_NAMEis unique and descriptive enough to avoid conflicts with other contracts.17-64: The
instantiatefunction has been updated to include logic for calculating thetriplestore_addressand saving it to theDataversestate. This is a significant change and should be thoroughly tested to ensure that the address calculation and state saving are functioning as expected.
- The use of
instantiate2_addressfunction to calculate thetriplestore_addressis correct and follows the CosmWasm documentation.- The conditional compilation with
#[cfg(not(test))]and#[cfg(test)]is a good practice to handle test scenarios differently from production code.- The
Responseobject is correctly constructed with attributes and messages, which is a good practice for debugging and transaction history.However, there is a potential issue with error handling:
- The
?operator is used afterset_contract_version,addr_canonicalize,query_wasm_code_info,instantiate2_address,addr_humanize, andDATAVERSE.savecalls. Ensure that theContractErrortype can properly handle the conversion fromStdErrorto provide meaningful error messages to the contract users.
- 83-154: The test
proper_instantiateis well-structured and covers the instantiation process of the contract. It checks the response attributes, messages, and the state after instantiation, which are key aspects to verify.
- Mocking the querier response with
update_wasmto return aCodeInfoResponseis a good practice for unit testing.- The use of
mock_envandmock_infoto simulate the environment and message info is correct.- The assertions are comprehensive and check for the expected values in the response and the state.
However, ensure that the
predicted addressused in the test matches the actual address that would be generated in a real environment to avoid discrepancies between test and production behavior.
amimart
force-pushed
the
feat/dataverse-instantiate
branch
from
e2cf607 to
89a55ee
Compare
Implement the instantiate message handling of the
dataversesmart contract according to #425.Details
To allow the
dataverseto manage its triple store (i.e. thecognitarium) I've added the necessary elements (i.e. code id & limitations) to its instantiation at the message level, without tainting it strongly ascognitariumas it could be another implementation.In its implementation, the instantiation of the triple store doesn't leverage a reply message mechanism to get its address but uses instead a generated predictable address, which causes some ugly necessary stuffs to make the tests works, see: CosmWasm/cosmwasm#1648.
Finally, the instantiated triple store smart contract takes as label the dataverse name suffixed by
_triplestore.Misc
A change in the build system has been necessary to allow compiling packages multiple times with different set of features, for instance: when running
cargo make wasmit should compilesokp4-cognitariumtwice, once with thelibraryfeature for theokp4-dataversecontract, and a second time for the cognitarium contract itself, but this not works.The solution here consists in the use of cargo-hack plugin to compiles each contract separately, the tradeoff is a longer wasm compilation time..
An issue can be tracked for this on cargo: rust-lang/cargo#4463
Summary by CodeRabbit
New Features
Dataversestructure to store dataverse information including atriplestore_address.triplestore_configto the instantiation message for configuring triple stores.TripleStoreConfigandTripleStoreLimitsInputstructures for better triple store configuration and limitation management.Documentation
TripleStoreConfigandStoreLimitsInput.Uint128andUint64.Bug Fixes
Instantiate2Addresserror variant.Tests
Chores
.size-limit.jsonto include new wasm file configurations.